Skip to content

[liqoctl] set gateway server Service location in peer command #2913

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Mar 6, 2025
Merged

[liqoctl] set gateway server Service location in peer command #2913

merged 2 commits into from
Mar 6, 2025

Conversation

@fra98
Copy link
Member

@fra98 fra98 commented Jan 31, 2025 •
edited
Loading

Description

This PR introduces the support to specify the gateway client and server location in a peering.
The --server-service-location flag in liqoctl peer allows to place the gateway server service in the provider (default) or consumer.
This can be useful if you cannot expose Services (or don't have LB UDP support) in the provider cluster or if the provider is behind a NAT (without going through the manual peering procedure or setting up the networking module individually).

@adamjensenbot
Copy link
Collaborator

Hi @fra98. Thanks for your PR!

I am @adamjensenbot.
You can interact with me issuing a slash command in the first line of a comment.
Currently, I understand the following commands:

  • /rebase: Rebase this PR onto the master branch (You can add the option test=true to launch the tests
    when the rebase operation is completed)
  • /merge: Merge this PR into the master branch
  • /build Build Liqo components
  • /test Launch the E2E and Unit tests
  • /hold, /unhold Add/remove the hold label to prevent merging with /merge

Make sure this PR appears in the liqo changelog, adding one of the following labels:

  • feat: 🚀 New Feature
  • fix: 🐛 Bug Fix
  • refactor: 🧹 Code Refactoring
  • docs: 📝 Documentation
  • style: 💄 Code Style
  • perf: 🐎 Performance Improvement
  • test: ✅ Tests
  • chore: 🚚 Dependencies Management
  • build: 📦 Builds Management
  • ci: 👷 CI/CD
  • revert: ⏪ Reverts Previous Changes

@github-actions github-actions bot added feat Adds a new feature to the codebase fix Fixes a bug in the codebase. docs Updates or adds documentation labels Jan 31, 2025
@fra98 fra98 changed the title fix: wggateway template ports when metrics not enabled [Liqoctl peer] set gateway server Service position Jan 31, 2025
@fra98 fra98 changed the title [Liqoctl peer] set gateway server Service position [liqoctl] set gateway server Service position in peer command Jan 31, 2025
@fra98 fra98 requested review from cheina97 and claudiolor January 31, 2025 14:55
@fra98
Copy link
Member Author

fra98 commented Jan 31, 2025

/rebase

@adamjensenbot adamjensenbot force-pushed the frt/liqoctl-gw-position branch from 91aad33 to cb2934c Compare January 31, 2025 16:45
@github-actions github-actions bot removed the fix Fixes a bug in the codebase. label Jan 31, 2025
@fra98
Copy link
Member Author

fra98 commented Jan 31, 2025

/test

@fra98 fra98 force-pushed the frt/liqoctl-gw-position branch from cb2934c to 9741cc1 Compare February 3, 2025 11:58
@fra98 fra98 requested a review from aleoli February 3, 2025 11:59
@fra98 fra98 changed the title [liqoctl] set gateway server Service position in peer command [liqoctl] set gateway server Service location in peer command Feb 3, 2025
@fra98
Copy link
Member Author

fra98 commented Feb 3, 2025

/test

@fra98
Copy link
Member Author

fra98 commented Feb 4, 2025

/test

@fra98 fra98 force-pushed the frt/liqoctl-gw-position branch from 9741cc1 to 2048e52 Compare February 4, 2025 12:02
@fra98
Copy link
Member Author

fra98 commented Feb 4, 2025

/test

1 similar comment
@fra98
Copy link
Member Author

fra98 commented Feb 5, 2025

/test

@fra98 fra98 force-pushed the frt/liqoctl-gw-position branch from 2048e52 to 62f1883 Compare February 5, 2025 15:01
@fra98
Copy link
Member Author

fra98 commented Feb 5, 2025

/test

@fra98
Copy link
Member Author

fra98 commented Feb 7, 2025

/rebase test=true

@adamjensenbot adamjensenbot force-pushed the frt/liqoctl-gw-position branch from 62f1883 to 00f95f2 Compare February 7, 2025 10:43
@frisso
Copy link
Member

frisso commented Feb 7, 2025

Difficult for me to understand what "server" means in the command line, "--server-service-location".
I would suggest to propose a more intuitive name (e.g., --gateway-server-location", perhaps?)

frisso
frisso reviewed Feb 7, 2025
View reviewed changes
frisso
frisso reviewed Feb 7, 2025
View reviewed changes
frisso
frisso reviewed Feb 7, 2025
View reviewed changes
@fra98 fra98 force-pushed the frt/liqoctl-gw-position branch 2 times, most recently from 56ce986 to c780282 Compare February 18, 2025 14:29
@fra98
Copy link
Member Author

fra98 commented Feb 18, 2025 •
edited
Loading

Difficult for me to understand what "server" means in the command line, "--server-service-location". I would suggest to propose a more intuitive name (e.g., --gateway-server-location", perhaps?)

Hi @frisso, the purpose of this flag is to allow the user to customize the location of the k8s Service that exposes the Gateway Server.
The rationale to keep the word service is that it's the most important thing to know for the common liqoctl peer user that does not need to know the internal of Liqo, nor what type of Server this is or what it does. For this flag, the thing that matters the most is the ability to expose a LoadBalancer Service (if not using NodePort ofc); therefore, the user should choose the cluster equipped with a working LoadBalancer provider.
Other reasons are:

  • coherence with other flags that customize the k8s Service (e.g,, --server-service-port, --server-service-loadbalancerip)
  • a more exhaustive name (e.g., --gateway-server-service-location) would be too verbose, and the flag description already indicates that the flag customizes the location of the Service exposing the Gateway Server.

@frisso
Copy link
Member

frisso commented Feb 18, 2025

Hello @fra98
As a user, I'm definitely confused by the string "--server-service". Service of what? You know that is the gateway service, but this is not really evident from the flag.

@fra98
Copy link
Member Author

fra98 commented Mar 3, 2025

/rebase test=true

@adamjensenbot adamjensenbot force-pushed the frt/liqoctl-gw-position branch from c780282 to 6e57717 Compare March 3, 2025 14:27
@fra98
Copy link
Member Author

fra98 commented Mar 3, 2025

@frisso
A good compromise between verbosity and clarity would be to add a short prefix (e.g., gw-) to the flag name.
In this case, a small refactoring is needed to change the other flags related to the gateway server/client, but I would leave that to a separate PR.

@fra98
Copy link
Member Author

fra98 commented Mar 3, 2025

/test

2 similar comments
@fra98
Copy link
Member Author

fra98 commented Mar 3, 2025

/test

@cheina97
Copy link
Member

cheina97 commented Mar 4, 2025

/test

@fra98
Copy link
Member Author

fra98 commented Mar 4, 2025

/rebase test=true

1 similar comment
@cheina97
Copy link
Member

cheina97 commented Mar 6, 2025

/rebase test=true

@adamjensenbot adamjensenbot force-pushed the frt/liqoctl-gw-position branch from 6e57717 to 708715b Compare March 6, 2025 15:01
@fra98
Copy link
Member Author

fra98 commented Mar 6, 2025

/merge

@cheina97
Copy link
Member

cheina97 commented Mar 6, 2025

/rebase

@cheina97
Copy link
Member

cheina97 commented Mar 6, 2025

/merge

@adamjensenbot adamjensenbot added the merge-requested Request bot merging (automatically managed) label Mar 6, 2025
@adamjensenbot adamjensenbot merged commit a1f8569 into liqotech:master Mar 6, 2025
14 checks passed
@adamjensenbot adamjensenbot added merge-requested Request bot merging (automatically managed) and removed merge-requested Request bot merging (automatically managed) labels Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@frisso frisso frisso left review comments

@claudiolor claudiolor claudiolor approved these changes

@aleoli aleoli aleoli approved these changes

@cheina97 cheina97 Awaiting requested review from cheina97

Assignees

No one assigned

Labels

docs Updates or adds documentation feat Adds a new feature to the codebase size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@fra98 @adamjensenbot @frisso @cheina97 @claudiolor @aleoli